chore(desktop): drop the completed hook transition section - #4332
Conversation
The renderer architecture ledger records zero `hookTransitions` entries, so the one-time Hook replacement escape hatch has no remaining consumers. Remove the config key, its schema validation, the transition accounting inside the monotonic debt check, and the tests that covered only that path. Every live debt ledger (legacy files, feature/platform imports, growth directories, AppShell files and closure, root debt and closure) is untouched, and the checker output is unchanged. Generated-by: Claude Code
jackwener
left a comment
There was a problem hiding this comment.
Reviewed the latest head be3cec0b2. No P0 or P1 — approving. Nothing at P2 or P3 either, so no inline comments. All checks green.
I came at this expecting to find a weakened guard and found the opposite. Removing 206 lines from an architecture checker is the shape that usually deserves suspicion, so I traced what hookTransitions actually did. It was an escape hatch, not an enforcement: it let a legitimate Hook swap be declared and then required the change to be paid one-for-one by removals of the named old Hook. Deleting it does not remove a check — it removes the only sanctioned way to grow a tracked Hook count.
The README confirms that reading. The rule goes from "a legitimate Hook replacement must use a one-time hookTransitions entry…" to "no tracked Hook call count may grow in a debt file." Simpler, absolute, and stricter than what it replaces.
The removal is complete and the mechanism was unused. renderer-architecture.json carried "hookTransitions": [] — an empty ledger, so nothing was relying on it — and no reference to hookTransitions survives anywhere in apps/desktop. The two deleted tests covered exactly the deleted behaviour (a paid replacement, and rejecting unconsumed or reused transitions), so they are moot rather than lost coverage. The validation that went with them — kebab-case ids, sort order, no duplicates, historical entries being immutable — was schema-checking for a config section that no longer exists.
One consequence worth being deliberate about, not an objection. With the escape hatch gone there is no declared path for a future Hook swap: growing useReducer while shrinking useState in the same debt file now simply violates. That is presumably the intent — debt files should shrink rather than churn — and it is stated plainly in the README, so a future reader will not be surprised. I mention it only so the tradeoff is a decision on record rather than a side effect: restoring the capability later means restoring roughly a hundred lines of ledger machinery.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the complete four-file change at exact head be3cec0b2443bd5c8d48bfa60118d928e89b9647. I found no P0-P3 issues.
The removed hookTransitions section was empty and had no surviving repository consumer. It was an explicit exception to the monotonic Hook-debt rule, not the rule itself: after this deletion, aggregate Hook debt growth and per-Hook growth remain rejected by validateMonotonicDebt, and the retained fixture exercises that boundary. Base-tree regeneration also safely drops the obsolete policy field while preserving the remaining growth-directory, root-debt, ownership, and dependency-migration policies.
Validation: clean Node 22 install, full build:test, full workspace typecheck, renderer architecture tests 60/60, the production architecture check against exact base ef94235, full lint, ASF-header audit, changed-file Biome, and git diff --check all passed. Hosted test and label checks are terminal green, and the PR is mergeable/clean.
Review notice: This review was prepared by an automated review agent operated by hqhq1025 and is published at the direction of AstroHan, who has read these findings and is the human accountable for them.
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head be3cec0b (verified unchanged at review time; label and test completed/success on this head).
Scaffolding removal, verified rather than assumed:
- The premise holds: the base ledger's
hookTransitionsis[](checked on the merge-base, not just on the PR's word), so every deleted branch — the per-entry validation, the transition diffing insidevalidateMonotonicDebt, the unconsumed-transition sweep — was provably unreachable. The checker's behavior on live debt ledgers is unchanged by construction. - Nothing else was swept up:
legacyRendererFiles,legacyFeatureImports, thelegacyAppShelldebt branches, androotDebtare all untouched, andallowsMigrationDependency'slegacyAppShellbranch stays because it serves the dependency-debt rule, not the removed transition path. Zero residual references tohookTransitions/HOOK_TRANSITION_SECTIONS/isTrackedHookNameat this head. - The surviving rule is the honest one: the README now states the plain no-new-hook rule instead of describing an escape hatch with no consumers.
Executed on a real Windows machine at this head: check-renderer-architecture.mjs passes against the real tree, and its test suite is 59/60 — the single failure (attests the canonical main source in the final Vite entry graph) is pre-existing and unrelated: it fails identically with the base versions of both files (I checked them out and re-ran), its POSIX /fixture/... paths never match on this platform, and the PR's diff does not touch that test.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
脚手架删除,全部核实而非假设:前提亲验——base 的 hookTransitions 确实是空数组,所以被删的校验/差分/清扫分支全都不可达,活账簿行为不变;其他债务账簿与 legacyAppShell 依赖分支原样保留;被删符号零残留。本机真 Windows:checker 对真实树通过;测试套件 59/60——唯一失败(Vite entry graph 证明)与 PR 无关:把 base 版文件换回来重跑同样挂(POSIX fixture 路径在本平台永不匹配),且 PR diff 未触碰该测试。
The renderer architecture ledger records zero `hookTransitions` entries, so the one-time Hook replacement escape hatch has no remaining consumers. Remove the config key, its schema validation, the transition accounting inside the monotonic debt check, and the tests that covered only that path. Every live debt ledger (legacy files, feature/platform imports, growth directories, AppShell files and closure, root debt and closure) is untouched, and the checker output is unchanged. Generated-by: Claude Code
Summary
apps/desktop/renderer-architecture.jsonrecorded zerohookTransitionsentries, so the one-time Hook replacement escape hatch had no remaining
consumers — the migration it existed for is finished. This removes the
scaffolding: the config key, its schema validation (
HOOK_TRANSITION_SECTIONS,the per-entry id/section/path/from/to/count checks, and the now-unused
isTrackedHookNamehelper), the transition accounting insidevalidateMonotonicDebt(base/new transition diffing, thedecreasesbookkeepingin the
hookCallsbranch, and the unconsumed-transition sweep), and the twotests that covered only that path. The renderer README paragraph describing the
escape hatch is reworded to state the plain rule that survives it.
Because the ledger was empty, every deleted branch was unreachable: the checker's
behavior and output are unchanged. All live debt ledgers are untouched —
legacyRendererFiles,legacyFeatureImports,legacyGrowthDirectories,legacyPlatformImports,legacyAppShell.files/legacyAppShell.closure,rootDebt, androotDebtClosure.allowsMigrationDependency'slegacyAppShell/legacyAppShellClosurebranch is also kept: it serves thedependency-debt rule, not hook transitions.
Verification
npm run check:renderer-architecture— passed, output identical tomainapart from no longer exercising hook transitions
node --test apps/desktop/scripts/check-renderer-architecture.test.mjs—60/60 pass
npm run format— clean, no fixes appliedAI use
Select exactly one:
Tool(s) and scope: Claude Code traced the
hookTransitionsdata flow, performedthe deletions, and drafted this description. Commit carries a
Generated-bytrailer.
Checklist
scaffolding and the tests that only covered it; the remaining 60 checker
tests are the regression guard
Does this PR entail a change in behavior?